Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

hostinfo_unix: Ignore tty(S|ACM) devices in TTY idle time calculation #4126

Merged
merged 1 commit into from
Feb 21, 2021
Merged

hostinfo_unix: Ignore tty(S|ACM) devices in TTY idle time calculation #4126

merged 1 commit into from
Feb 21, 2021

Conversation

Flowdalic
Copy link
Contributor

@Flowdalic Flowdalic commented Dec 12, 2020 •
edited
Loading

The atime (access time) of device nodes whose name starts with
/dev/ttyS (serial port) or /dev/ttyACM (serial USB) should not be used
for TTY idle time calculation. It is perfectly possible that they are
used without any user being active.

Previously BOINC would not run on some of our systems, because those systems also had a monitoring device connected via a serial port. Since this monitoring device is periodically checked, BOINC would assume that a user is active. I think it is sensible to exclude ttyS and ttyACM (and probably also ttyUSB devices. But those I couldn't test as we do not have one of those) from TTY idle time calculation.

@Flowdalic Flowdalic force-pushed the ignore-tty-S-ACM branch 2 times, most recently from 8a8088a to cfba160 Compare December 14, 2020 08:07
@AenBleidd
Copy link
Member

@Flowdalic, please remove e37ec47 because we're not support c++17 for all platforms and cfba160 as not related to this PR

@Flowdalic
Copy link
Contributor Author

@Flowdalic, please remove e37ec47 because we're not support c++17 for all platforms

I did so, but I am still battling issues caused by the build not even supporting C++11. May I ask which platforms prevent the upgrade to C++17 (or C++11)? At least C++11 should be widely available by now and makes the code a lot easier to read and write. I could live without C++17, but C++11 would be really nice to have.

@Flowdalic Flowdalic mentioned this pull request Dec 16, 2020
Closed
@AenBleidd
Copy link
Member

@Flowdalic, currently Windows build is the last that is not support C++11 or later. We have planned move to c++11 (or later) but since it's not decided yet, we should still not use any C++11 features. It shouldn't be a big deal to not use any c++11 features in the code because your change isn't big

@Flowdalic
Copy link
Contributor Author

It shouldn't be a big deal to not use any c++11 features in the code because your change isn't big

Sure, if you are experienced with pre-C++11 coding. But right now I have to find out how to replace my modern idioms with pre-C++11 ones. And additionally, it appears I can only become aware of used C++11 idioms in my code via BOINC's CI, as the build may succeeds locally but fails in the CI because of some CI jobs not supporting C++11. I am not sure, but this, in turn, is probably because BOINC's build system does not specify a C++ standard version (e.g. --std=c++03).

I do not know how much work it would be to switch BOINC to C++11, but I would suggest that you consider it, because frankly, as "drive-by spare-time contributor" I am not really motivated to code pre-C++11.

@Flowdalic Flowdalic mentioned this pull request Dec 16, 2020
Closed
@AenBleidd
Copy link
Member

I do not know how much work it would be to switch BOINC to C++11, but I would suggest that you consider it, because frankly, as "drive-by spare-time contributor" I am not really motivated to code pre-C++11.

And this definitely will be done soon

@ChristianBeer
Copy link
Member

Close and reopen to trigger Github Actions build

@ChristianBeer ChristianBeer reopened this Dec 31, 2020
@ChristianBeer
Copy link
Member

Close and reopen to trigger Github Actions build and creation of artifacts

@ChristianBeer ChristianBeer reopened this Jan 14, 2021
@Flowdalic
Copy link
Contributor Author

I have force pushed this PR back to 8a8088a, i.e. the initial version of this PR. This commit uses a "fancy" for-each loop to iterate over a std::vector, which was previously not possible to use in BOINC, since BOINC used an older C++ standard where for-each loops where not possible.

@AenBleidd
Copy link
Member

@BOINC/committer, please don't merge it right now

AenBleidd
AenBleidd requested changes Jan 14, 2021
View reviewed changes
client/hostinfo_unix.cpp Outdated Show resolved Hide resolved
client/hostinfo_unix.cpp Outdated Show resolved Hide resolved
@AenBleidd AenBleidd self-assigned this Jan 14, 2021
@codecov
Copy link

codecov bot commented Jan 14, 2021 •
edited
Loading

Codecov Report

Merging #4126 (ed70369) into master (57fa9d5) will decrease coverage by 10.25%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master   #4126       +/-   ##
============================================
- Coverage     16.66%   6.40%   -10.26%     
  Complexity      752     752               
============================================
  Files           134     276      +142     
  Lines         13131   34902    +21771     
  Branches       1492    1492               
============================================
+ Hits           2188    2235       +47     
- Misses        10827   32551    +21724     
  Partials        116     116
Impacted Files Coverage Δ Complexity Δ
sched/sched_limit.h 12.28% <0.00%> (-87.72%) 0.00% <0.00%> (ø%)
lib/msg_log.h 14.28% <0.00%> (-85.72%) 0.00% <0.00%> (ø%)
lib/sched_msgs.h 66.66% <0.00%> (-33.34%) 0.00% <0.00%> (ø%)
lib/sched_msgs.cpp 4.34% <0.00%> (-5.66%) 0.00% <0.00%> (ø%)
lib/msg_log.cpp 9.52% <0.00%> (-0.12%) 0.00% <0.00%> (ø%)
lib/str_util.cpp 55.61% <0.00%> (-0.08%) 0.00% <0.00%> (ø%)
lib/url.cpp 92.85% <0.00%> (-0.06%) 0.00% <0.00%> (ø%)
db/db_base.cpp 0.76% <0.00%> (-0.01%) 0.00% <0.00%> (ø%)
sched/sched_config.cpp 0.34% <0.00%> (-0.01%) 0.00% <0.00%> (ø%)
lib/util.h 0.00% <0.00%> (ø) 0.00% <0.00%> (ø%)
... and 160 more

AenBleidd
AenBleidd requested changes Jan 15, 2021
View reviewed changes
client/hostinfo_unix.cpp Outdated Show resolved Hide resolved
client/hostinfo_unix.cpp Outdated Show resolved Hide resolved
@Flowdalic
hostinfo_unix: Ignore tty(S|ACM) devices in TTY idle time calculation
ed70369 
The atime (access time) of device nodes whose name starts with
/dev/ttyS (serial port) or /dev/ttyACM (serial USB) should not be used
for TTY idle time calculation. It is perfectly possible that they are
used without any user being active.
@AenBleidd
Copy link
Member

@RichardHaselgrove, could you please test this?

@RichardHaselgrove
Copy link
Contributor

@AenBleidd - not quite sure what you want me to test? I've installed
linux_client_ed70369c091923addf5a8d64bed92c0417e91e07
and set PrivateTmp=true again (the conflicting project has no work at the moment). Idle detection works as intended.

This machine has mouse and keyboard connected via a single USB connection (KVM switch acts as USB hub). Idle time debug log counts upwards as expected, and restarts computation after I've kept my hand off the mouse for three minutes. I can reset the settings to run always over a remote RPC from upstairs, without interrupting work again.

But I don't have any other TTY or emulated-TTY devices connected, so I can't tell if they're ignored now.

@AenBleidd
Copy link
Member

But I don't have any other TTY or emulated-TTY devices connected, so I can't tell if they're ignored now.

At least normal USB mouse is not ignored. That's enough as for me

@AenBleidd AenBleidd merged commit 1ad473e into BOINC:master Feb 21, 2021
@Flowdalic Flowdalic deleted the ignore-tty-S-ACM branch April 14, 2023 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AenBleidd AenBleidd AenBleidd approved these changes

@davidpanderson davidpanderson Awaiting requested review from davidpanderson

Assignees

@davidpanderson davidpanderson

Labels
C: Client OS: Linux P: Minor PR: Enhancement
Projects
None yet
Milestone
Client Release 7.20.0
Development

Successfully merging this pull request may close these issues.
5 participants
@Flowdalic @AenBleidd @ChristianBeer @RichardHaselgrove @davidpanderson